-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
timers: run nextTicks after each immediate and timer #22842
timers: run nextTicks after each immediate and timer #22842
Conversation
Ok, looks like Stress test: https://ci.nodejs.org/job/node-stress-single-test/2014/ and against master: https://ci.nodejs.org/job/node-stress-single-test/2015/ Edit: Also |
Can we get some benchmarks to show just what kind of performance hit we should expect from this? |
@mscdex Yup, I'm hoping to first figure out if we can get all the tests to pass and CitGM to work before I go down the road of trying to figure out performance and maybe optimize that. |
@apapirovski had to abort the CitGM run to restart Jenkins, two workers didn't finish. Can you restart it if needed? I started the master run after rebooting, link above is correct. |
lib/timers.js
Outdated
@@ -264,7 +264,7 @@ function listOnTimeout(list, now) { | |||
|
|||
debug('timeout callback %d', msecs); | |||
|
|||
var diff, timer; | |||
var diff, ran, timer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ran what? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kno, I kno 😆 I'll make it more descriptive. But to answer the question: an iteration of the loop right after it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= false
?
There are currently 3 test failures. They are:
|
e7b20ce
to
82e1515
Compare
CI: https://ci.nodejs.org/job/node-test-pull-request/17179/ I believe that all 3 test failures above were related to incorrect error resolution inside |
Should we add some documents about this behavior change? |
I think we're in the clear on tests now so here's benchmarks: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/238/ I will be working on new tests for this behaviour and documentation later today or tomorrow. I'll also follow up with perf improvements as necessary. Results:
Given the perf improvements here #20555, I think we can live with the |
let refCount = 0; | ||
if (queue !== outstandingQueue) { | ||
queue.head = queue.tail = null; | ||
immediateInfo[kHasOutstanding] = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could leave a comment here about how this = null
thing works? it's... pretty difficult to read/understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should disallow that in the lint rules. please split it into two.
lib/timers.js
Outdated
if (ran) | ||
runNextTicks(); | ||
else | ||
ran = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on comments here and above where we use the same pattern, easy to misunderstand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me, but it certainly confusing, implementation-wise.
We should probably CITGM this, it seems slightly ecosystem-risky to me? Don't think it's necessarily major
by default though.
Edit: Re - benchmarks; maybe we can do some extra optimization there. I'll try to take profiles.
oof, this change is.. kinda expensive. Here are the numbers I got for
Profiles: |
^ that seems worse than the benchmark CI run. But it's not generally surprising, it now has to run nextTicks on each timer which necessitates crossing over to C++ for each timer. The question is how frequent of an occurrence this is and whether this is a sacrifice we're willing to make for correctness. |
Perhaps running these changes against some real-world scenarios (e.g. acmeair, etc.) might help answer some of this? |
Promise-based code is the most likely candidate here. the question is, do people really do lots of async promise stuff in timeouts? I don't really know but I do feel like I usually don't do much async stuff in timeouts. |
I'm planning to revisit this again over the weekend. Hopefully tidy up some of the perf and get some clearer practical results. |
82e1515
to
ff24a20
Compare
New benchmarks after more optimizations: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/245/ |
88792fe
to
b6e4f68
Compare
I think this is a good idea, it simplifies the API and makes it much more consistent. nodejs/nodejs.org#1804 for example describes some confusion about when the tick queue is cleared, and some attempts to make the docs describe the behaviour. Does this PR clear the tick queue only after timer callbacks, or does it do so after immediates are run, too? Both would be useful, IMO. |
Landed in e7af983 |
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
In order to better match the browser behaviour, run nextTicks (and subsequently the microtask queue) after each individual Timer and Immediate, rather than after the whole list is processed. The current behaviour is somewhat of a performance micro-optimization and also partly dictated by how timer handles were implemented. PR-URL: #22842 Fixes: #22257 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Check that $TRAVIS_COMMIT_RANGE is set before trying to lint commit messages in Travis CI. Refs: #23572 (comment) Refs: #22842 (comment) PR-URL: #23725 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Node.js v11.x introduced a difference with how timers are run that results in some about ids in the blacklisted tests being off. Needs investigation but, regardless, should be fixed by T206222 See nodejs/node#22842 Bug: T206222 Change-Id: I9f0f906390d763ea52c61c9ae586da13172d687e
I tried to clarify how this behaves before this change landed in v11 nodejs/nodejs.org#1804 |
It was a little unclear for me when the nextTickQueue is processed (see this StackOverflow question). After some experimentation and more googling, re-write to make it more clear.
Deduplication section appears to be describing how things worked prior to nodejs/node#22842, so is no longer correct. The output in 11.12 is ``` setImmediate 1 process.nextTick a process.nextTick b setImmediate 2 process.nextTick a process.nextTick b ```
@apapirovski:Would you mind reviewing the article nodejs/nodejs.org#2158, if you are free |
…-nexttick.md' (#2158) Deduplication section appears to be describing how things worked prior to nodejs/node#22842, so is no longer correct.
In order to better match browser behaviour, run nextTicks (and subsequently the microtask queue) after each individual Timer and Immediate, rather than after the whole list is processed. The current behaviour is somewhat of a performance micro-optimization and also partly dictated by how timer handles were implemented.
This will have a significant impact on how promises are resolved/rejected (due to changes to the microtask queue timing) which is IMO a reasonably important change.
@Fishrock123 already started some work on this in #22316 but I spun this off into a separate PR as I had good understanding of what needed to happen to actually pass the tests.
I still likely need to update the docs and write a few tests. This is also definitely
semver-major
. This will also need a CitGM run.Fixes: #22257
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes